Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Gutenberg] Autosave #12489

Merged
merged 5 commits into from
Sep 27, 2019
Merged

[Gutenberg] Autosave #12489

merged 5 commits into from
Sep 27, 2019

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Sep 6, 2019 •
edited
Loading

This PR implements the Gutenberg autosave mechanism replacing the previously implemented client-side autosave.

Update: The new logic implemented is described on #12489 (comment)
Updated Gutenberg PR: WordPress/gutenberg#17548

To test (updated):

  • Run metro server on gutenberg-mobile:develop
  • Create/open a Gutenberg post.
  • Write some content.
    • You can check if editorDidAutosave() is invoked on every content change.
    • Check that debouncerCallback is invoked after 1 second of inactivity.
    • Check that debouncerCallback is invoked after typing 50 characters without break.
  • Kill the app and restart it again.
  • Check that the post was automatically saved.

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@etoledom etoledom added the Gutenberg Editing and display of Gutenberg blocks. label Sep 6, 2019
@etoledom etoledom added this to the 13.3 milestone Sep 6, 2019
@etoledom etoledom requested a review from koke September 6, 2019 09:04
@etoledom etoledom self-assigned this Sep 6, 2019
@daniloercoli daniloercoli mentioned this pull request Sep 6, 2019
Merged
1 task
koke
koke reviewed Sep 6, 2019
View reviewed changes
Copy link
Member

@koke koke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and it works OK, however the behavior is different than it used to be. Previously, the debouncer logic would ensure that we autosaved at most once every 5 seconds. Now, it seems it will autosave after 2 seconds of inactivity.

I think the previous behavior was safer, as it ensured that if anything crashed you would lose at most 5 seconds of work. Now you could create a lot of content without pausing, and so without autosaving, and lose it all if the app crashes.

Also, let's keep in mind that the autosave logic in the web is meant to autosave to the server and AFAIK there's no local autosave concept (although it would be a nice feature addition 😉)

@daniloercoli daniloercoli mentioned this pull request Sep 20, 2019
Merged
2 tasks
@mchowning
Copy link
Contributor

@etoledom : do we want this to target the (just created) gb/release-1.13.0 branch?

@etoledom etoledom changed the base branch from develop to gb/release-1.13.0 September 20, 2019 17:22
@etoledom
Copy link
Contributor Author

@etoledom : do we want this to target the (just created) gb/release-1.13.0 branch?

Sounds good! Just did the change 👍

@etoledom
Copy link
Contributor Author

I think the previous behavior was safer, as it ensured that if anything crashed you would lose at most 5 seconds of work. Now you could create a lot of content without pausing, and so without autosaving, and lose it all if the app crashes.

I see what you mean. We could always reduce the inactivity time to 1 second? I'd guess it's quite a lot less likely for someone to write a big piece of content without any one second of inactivity.

@daniloercoli - what do you think?

@daniloercoli
Copy link
Contributor

I see what you mean. We could always reduce the inactivity time to 1 second? I'd guess it's quite a lot less likely for someone to write a big piece of content without any one second of inactivity.

@daniloercoli - what do you think?

Yes, we can change it to 1 sec of inactivity, or even set a timer, or follow another logic we think is more appropriate for a mobile app with local storage. With this PR we're adding the autosaving to mobile-gb, before this PR nothing was autosaved, so I think there are room for improvement in another PR.

@etoledom
Copy link
Contributor Author

With this PR we're adding the autosaving to mobile-gb, before this PR nothing was autosaved, so I think there are room for improvement in another PR.

We did have a timer on WPiOS requesting the current content to save it. That's the reason of @koke's comment here.

we can change it to 1 sec of inactivity, or even set a timer, or follow another logic we think is more appropriate for a mobile app with local storage.

Is nice that we have that flexibility. I would say to merge this change on WPiOS as it is now to use the same mechanism as WPAndroid, and as @daniloercoli mentioned, keep improving it. 👍

What do you think @koke ?

@jtreanor jtreanor modified the milestones: 13.3 ❄️, 13.4 Sep 24, 2019
@etoledom etoledom changed the base branch from gb/release-1.13.0 to develop September 26, 2019 06:39
@etoledom
Merge remote-tracking branch 'origin/develop' into issue/gutenberg-au…
18f5a63 
…tosave
@etoledom
Editors: Implement new Autosave logic
ada7cd9 
Autosave is now triggered after 50 continuous content changes or after 1 second of inactivity. This in both Aztec and Gutenberg.
etoledom
etoledom commented Sep 26, 2019
View reviewed changes
WordPress/Classes/ViewRelated/Post/Autosaver.swift
/// - Parameter delay: Maximum time of inactivity before autosaving. Default 1 second.
/// - Parameter action: The action to be triggered when autosave is fired.
///
init(changesThreshold: Int = 50, delay: Double = 1, action: @escaping () -> Void) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaults are taken from WPAndroid.

@etoledom
Copy link
Contributor Author

etoledom commented Sep 26, 2019 •
edited
Loading

Update:

We have decided to implement a different approach for autosaving. This is to send content change events from Gutenberg, and let the native apps decide when is best to do an autosave.

We also changed the logic to decide when is best to autosave:

  • We will wait for 1s of inactivity, or
  • We will autosave after 50 continuous changes.

This new autosave logic has been implemented in both Gutenberg and Aztec, matching the logic on WPAndroid.

I have updated the initial PR description.
Note that it's still needed to run Metro server on gutenebrg-mobile:develop to properly test this PR.

cc @daniloercoli @maxme

SergioEstevao
SergioEstevao approved these changes Sep 27, 2019
View reviewed changes
Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working great @etoledom ! Excellent to see that you also changed Aztec editor and sharing the code between the two editors.

@etoledom etoledom merged commit 91b39cc into develop Sep 27, 2019
@etoledom etoledom deleted the issue/gutenberg-autosave branch September 27, 2019 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergioEstevao SergioEstevao SergioEstevao approved these changes

@koke koke Awaiting requested review from koke

Assignees

@etoledom etoledom

Labels
Gutenberg Editing and display of Gutenberg blocks.
Projects
None yet
Milestone
13.4
Development

Successfully merging this pull request may close these issues.
6 participants
@etoledom @mchowning @daniloercoli @koke @SergioEstevao @jtreanor